Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds project scaffolding for the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
vitest.config.ts (1)
7-7: Consider enabling test isolation for more reliable tests.Setting
isolate: falsedisables test isolation, which can lead to shared state between tests and make them order-dependent. This may cause flaky tests and make debugging harder. Consider enabling isolation unless there's a specific performance reason to disable it.🔎 Proposed change
- isolate: false, + isolate: true,eslint.config.js (1)
9-11: Reconsider disabling strict TypeScript rules.Disabling
no-explicit-any,no-var-requires, andno-require-importsweakens TypeScript's type safety guarantees. Specifically:
no-explicit-anyallows uncheckedanytypes, reducing type safetyno-var-requiresandno-require-importspermit CommonJS patterns, which conflicts with the ESM-only build configuration intsup.config.tsConsider enabling these rules to maintain stricter type checking and module consistency.
.github/workflows/publish.yaml (1)
29-33: Version update logic works but could be more robust.The
sedcommand for updating the version is functional but relies on the exact string format insrc/common/version.ts. While this is acceptable for now, consider using a more robust approach like a Node.js script if the version management becomes more complex.tsconfig.json (1)
12-13: Decorator options enabled but not used.The
emitDecoratorMetadataandexperimentalDecoratorsoptions are enabled, but no decorators appear to be used in the provided code. While this doesn't cause issues, these options may be unnecessary unless decorators are planned for future use.src/hono/index.ts (1)
1-4: Clean public API surface.The exports are well-organized, separating type-only exports from value exports. The
.jsextensions are appropriate for ESM compatibility.Consider whether
getConsumerfrom./utils.jsshould also be exported for users who may want to read the current consumer value in their handlers.tests/utils.ts (1)
3-28: Consider adding error handling for malformed JSON.The function correctly decompresses and decodes the logged data. However, if the decompressed data is not valid JSON,
JSON.parsewill throw an uncaught exception.For test utilities, this may be acceptable since a failure would surface as a test error, but wrapping in try-catch could provide clearer error messages.
🔎 Optional: Add error context for debugging
const jsonString = decoder.decode(decompressed); - - return JSON.parse(jsonString); + try { + return JSON.parse(jsonString); + } catch (error) { + throw new Error(`Failed to parse logged data: ${jsonString.slice(0, 100)}...`); + } }tests/hono/app.ts (1)
8-60: Test app is well-structured for comprehensive middleware testing.The app covers key scenarios: query validation, path params, JSON body, error handling, and streaming responses.
Minor observation:
getAppis declaredasyncbut contains noawaitexpressions. This could be simplified to a synchronous function.🔎 Optional: Remove unnecessary async
-export const getApp = async () => { +export const getApp = () => { const app = new Hono(); // ... return app; };tests/hono/app.test.ts (1)
33-106: Good test coverage for core middleware behavior.The tests effectively validate:
- Request/response logging structure
- Path parameter resolution (
/hello/:idlogged as template, not/hello/123)- Request body and headers capture
- Error response handling
Consider adding coverage for:
- The
/streamendpoint (streaming responses)- The
consumerfield in logged data (set viasetConsumer(c, "test")in the/hellohandler)src/hono/utils.ts (1)
35-41: Consider adding a brief comment explaining the silent catch.The empty catch block is intentional for environments where
executionCtx.waitUntilis unavailable (e.g., non-Cloudflare Workers). A brief comment would improve maintainability.Suggested documentation
export function tryWaitUntil(c: Context, promise: Promise<unknown>) { try { c.executionCtx.waitUntil(promise); } catch (error) { - // ignore + // Silently ignore - executionCtx.waitUntil may not be available in all environments } }src/common/headers.ts (1)
1-9: Consider exportingSUPPORTED_CONTENT_TYPESfor testing and documentation.The constant is currently not exported, which limits testability and makes it harder for consumers to understand what content types are supported.
Suggested change
-const SUPPORTED_CONTENT_TYPES = [ +export const SUPPORTED_CONTENT_TYPES = [src/common/consumers.ts (1)
7-15: Consider the implications of unbounded cache growth and hash collisions.The
seenConsumerHashesSet persists for the lifetime of the module/function instance and is never cleared. While serverless instances are typically short-lived, warmed instances or edge runtimes could accumulate entries over time.Additionally, the 32-bit hash space means collisions become likely at scale (~77K unique identifier+name+group combinations gives ~50% collision probability). A collision would cause a consumer object to be incorrectly reduced to just its identifier string.
If this is intentional for the expected scale, consider adding a brief comment documenting these design tradeoffs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
.github/workflows/publish.yaml(1 hunks).github/workflows/summary.yaml(1 hunks).github/workflows/tests.yaml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)LICENSE(1 hunks)README.md(2 hunks)eslint.config.js(1 hunks)package.json(1 hunks)renovate.json(1 hunks)src/common/bytes.ts(1 hunks)src/common/config.ts(1 hunks)src/common/consumers.ts(1 hunks)src/common/headers.ts(1 hunks)src/common/masking.ts(1 hunks)src/common/output.ts(1 hunks)src/common/response.ts(1 hunks)src/common/version.ts(1 hunks)src/hono/index.ts(1 hunks)src/hono/middleware.ts(1 hunks)src/hono/utils.ts(1 hunks)tests/common/consumers.test.ts(1 hunks)tests/common/headers.test.ts(1 hunks)tests/common/masking.test.ts(1 hunks)tests/common/response.test.ts(1 hunks)tests/hono/app.test.ts(1 hunks)tests/hono/app.ts(1 hunks)tests/utils.ts(1 hunks)tsconfig.json(1 hunks)tsup.config.ts(1 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
tests/common/consumers.test.ts (1)
src/common/consumers.ts (1)
consumerFromStringOrObject(17-43)
tests/hono/app.ts (3)
src/hono/middleware.ts (1)
useApitally(17-96)src/hono/index.ts (2)
useApitally(3-3)setConsumer(4-4)src/hono/utils.ts (1)
setConsumer(24-29)
tests/common/response.test.ts (2)
src/common/response.ts (1)
captureResponse(16-94)src/common/bytes.ts (2)
stringToBytes(1-3)bytesToString(5-7)
src/hono/utils.ts (1)
src/common/consumers.ts (2)
ApitallyConsumer(1-5)consumerFromStringOrObject(17-43)
src/hono/middleware.ts (8)
src/common/config.ts (2)
ApitallyConfig(1-10)mergeConfigWithDefaults(23-27)src/common/masking.ts (1)
DataMasker(38-147)src/common/response.ts (1)
captureResponse(16-94)src/common/headers.ts (3)
isSupportedContentType(50-55)parseContentLength(31-48)convertHeaders(11-29)src/common/bytes.ts (1)
stringToBytes(1-3)src/hono/utils.ts (3)
listEndpoints(8-22)getConsumer(31-33)tryWaitUntil(35-41)src/common/version.ts (1)
VERSION(1-1)src/common/output.ts (1)
logData(53-63)
tests/common/headers.test.ts (1)
src/common/headers.ts (3)
parseContentLength(31-48)convertHeaders(11-29)isSupportedContentType(50-55)
src/common/response.ts (1)
src/common/bytes.ts (2)
concatBytes(9-18)stringToBytes(1-3)
tests/common/masking.test.ts (3)
src/common/output.ts (1)
OutputData(4-27)src/common/bytes.ts (2)
stringToBytes(1-3)bytesToString(5-7)src/common/masking.ts (1)
DataMasker(38-147)
src/common/consumers.ts (1)
src/hono/index.ts (1)
ApitallyConsumer(2-2)
tests/hono/app.test.ts (3)
tests/hono/app.ts (1)
getApp(8-61)tests/utils.ts (2)
wait(30-32)getLoggedData(3-28)src/common/bytes.ts (2)
bytesToString(5-7)base64ToBytes(24-31)
src/common/output.ts (2)
src/common/consumers.ts (1)
ApitallyConsumer(1-5)src/common/bytes.ts (1)
bytesToBase64(20-22)
src/common/masking.ts (3)
src/common/config.ts (1)
ApitallyConfig(1-10)src/common/output.ts (1)
OutputData(4-27)src/common/bytes.ts (2)
bytesToString(5-7)stringToBytes(1-3)
🔇 Additional comments (45)
vitest.config.ts (1)
8-13: LGTM!The coverage configuration is well-structured, appropriately excluding type definition files and generating both text and LCOV reports.
.gitignore (1)
1-8: LGTM!The gitignore patterns appropriately cover build artifacts, dependencies, editor configurations, and debug logs.
renovate.json (1)
1-4: LGTM!The Renovate configuration correctly references the schema and extends the shared organizational configuration for consistent dependency management.
tsup.config.ts (1)
3-12: Verify thatbundle: falseis the intended behavior.The configuration sets
bundle: false, which means external dependencies won't be bundled into the output. This is typically correct for libraries where users install dependencies separately, but ensure this aligns with the package distribution strategy.The rest of the configuration looks appropriate:
- ESM format is modern and widely supported
- Type declarations and sourcemaps will aid consumers
platform: "neutral"is correct for cross-runtime compatibility.github/workflows/summary.yaml (2)
9-11: LGTM!The permissions are appropriately scoped to read-only access for checks, following the principle of least privilege.
13-16: No action required—version is current with no known security advisories.The workflow uses
poseidon/[email protected], which is the latest available version. No published security advisories were found for this action.eslint.config.js (1)
12-18: LGTM!The
no-unused-varscustomization is well-configured, appropriately ignoring single-underscore parameters and common error variable names while still catching genuinely unused variables..pre-commit-config.yaml (1)
1-7: pre-commit-hooks v6.0.0 is current.v6.0.0 is the latest version and the hook selection is appropriate for maintaining consistent file formatting. No action required.
package.json (1)
1-78: LGTM!The package configuration is well-structured with proper ESM exports, optional peer dependencies, and comprehensive metadata. The subpath exports pattern (
./hono) aligns well with supporting multiple frameworks in the future..github/workflows/tests.yaml (2)
1-16: LGTM!The workflow structure is well-organized with appropriate separation of concerns (pre-commit checks, type validation with attw, and test coverage). The concurrency configuration will prevent redundant runs.
Also applies to: 20-55
17-19: Invalid Python version specified.Python 3.14 has not been released yet. The latest stable Python version is 3.13. Using an unreleased version will cause the workflow to fail or use an unstable pre-release.
🔎 Proposed fix
- uses: actions/setup-python@v6 with: - python-version: "3.14" + python-version: "3.13"Likely an incorrect or invalid review comment.
README.md (1)
19-19: LGTM!Badge and documentation link updates are appropriate for the new project structure.
Also applies to: 67-67
tests/common/response.test.ts (1)
1-37: LGTM!The test coverage appropriately validates the core response capture functionality, including the important edge case where the body exceeds the size limit.
tests/common/headers.test.ts (1)
1-46: LGTM!Comprehensive test coverage for header utilities with good edge case handling including undefined, null, arrays, and type conversions.
.github/workflows/publish.yaml (1)
1-28: LGTM!The workflow structure is well-organized with proper checks and tests before publishing. The permissions are correctly scoped.
Also applies to: 34-37
tsconfig.json (1)
1-11: LGTM!The TypeScript configuration is well-structured with strict mode enabled and proper module resolution for NodeNext. The configuration appropriately covers both source and test files.
Also applies to: 14-15
tests/common/consumers.test.ts (1)
1-26: Well-structured test coverage for consumer utility.The test effectively validates:
- Null/empty/whitespace handling returning
undefined- String normalization
- Object processing with identifier-only, identifier+name, and identifier+name+group scenarios
- Deduplication behavior via the hash-based caching (lines 12-17 correctly test that the second call with same identifier/name returns just the identifier)
tests/utils.ts (1)
30-32: LGTM!Simple and effective delay utility with a sensible default.
src/common/config.ts (1)
1-27: Configuration structure is well-designed.The type definition is clear and the defaults are sensible:
enabled: falserequires explicit opt-in- Response headers logged by default, request headers/bodies opt-in
Note that
mergeConfigWithDefaultsperforms a shallow merge, so user-provided arrays (maskHeaders,maskBodyFields,excludePaths) completely replace the defaults rather than appending. Since defaults are empty arrays, this is correct behavior.src/common/bytes.ts (1)
1-18: LGTM!The remaining utilities (
stringToBytes,bytesToString,concatBytes,base64ToBytes) are correctly implemented using web standard APIs.src/hono/middleware.ts (2)
23-95: Middleware structure is well-designed.The overall pattern is solid:
- Response capture via
TransformStreamallows non-blocking body collection- Deferred logging via
tryWaitUntilworks well for serverless environments- Proper masking applied before logging
- Consumer extraction supports both string and object forms
The response time measurement (line 37, 81) includes the full response streaming duration. This is appropriate for total request handling time rather than TTFB.
40-47: The code works as intended. Hono caches request bodies, allowingc.req.arrayBuffer()to be called even after downstream handlers have consumed the body via methods likec.req.json(). The test "Logs POST request with JSON body" confirms this scenario works correctly.tests/common/masking.test.ts (5)
20-34: Partial override for nested objects may not work as intended.The spread operator
...overridesonly performs a shallow merge. When you pass{ request: { path: "/healthz" } }(line 43), it completely replaces the defaultrequestobject, losingheadersandbodydefaults. This is likely intentional for some tests, but worth noting for clarity.For test cases like line 43, this means
data1.requestwill only havepathand noheadersorbody, which aligns with testing the exclude path behavior. The current approach is acceptable since each test explicitly provides what it needs.
37-58: LGTM!The test correctly validates built-in path exclusions, custom exclusions via regex, and non-excluded paths. The assertions properly verify that excluded paths clear headers, body, and set the exclude flag.
60-82: LGTM!Tests correctly verify that headers and bodies are not logged when their respective config flags are disabled.
84-132: LGTM!Header and body masking tests thoroughly cover built-in patterns (authorization, password, token), custom patterns, nested objects, and arrays. Good coverage of the masking functionality.
134-173: LGTM!NDJSON and non-JSON body tests correctly validate that NDJSON lines are individually parsed and masked, while plain text bodies remain unchanged.
src/hono/utils.ts (2)
8-22: LGTM!The
listEndpointsfunction correctly filters out middleware handlers and HTTP methods that typically don't need tracking (ALL, HEAD, OPTIONS). The uppercase normalization ensures consistent method names.
24-33: LGTM!
setConsumerandgetConsumerproperly handle the consumer lifecycle, withgetConsumerdelegating toconsumerFromStringOrObjectfor normalization and deduplication.src/common/headers.ts (3)
11-29: LGTM!
convertHeaderscorrectly handles all input types: undefined,Headersinstance, and plain objects with string/array/number values. The flatMap approach elegantly handles multi-value headers.
31-48: LGTM!
parseContentLengthdefensively handles all expected input types including the recursive case for string arrays. TheisNaNcheck properly guards against unparseable strings.
50-55: LGTM!
isSupportedContentTypecorrectly usesstartsWithwhich handles content-type parameters (e.g.,application/json; charset=utf-8will matchapplication/json).src/common/masking.ts (4)
5-30: LGTM!The built-in patterns provide good coverage for common sensitive paths, headers, and body fields. The regex patterns are appropriately case-insensitive.
67-83: LGTM!The recursive
maskBodymethod correctly handles nested objects and arrays. Only string values matching sensitive field patterns are masked, which preserves data structure integrity.
111-137: Consider the assumption that missing content-type implies JSON.Line 115 treats bodies with no content-type as JSON (
!contentType || /\bjson\b/i.test(contentType)). While the outer try-catch safely handles parse failures, this could cause unexpected behavior or unnecessary parsing attempts for non-JSON bodies without a content-type header.This is likely acceptable since:
- The parse failure is caught and body is left unchanged
- Most APIs set content-type headers
However, verify this assumption aligns with your expected use cases.
139-146: LGTM!Header masking is applied last, after body processing, and correctly respects the logging configuration flags.
src/common/output.ts (3)
4-27: LGTM!The
OutputDatatype is well-structured with appropriate optional fields for flexible logging scenarios.
40-51: Note: Input data is mutated.The function adds a
toJSONproperty to the bodyUint8Arrays, mutating the originalOutputData. This is acceptable since the data is typically used once for logging, but worth noting if the data might be reused.
53-63: LGTM!The
logDatafunction correctly implements a fallback strategy when the log message exceeds the Cloudflare Workers limit, stripping bodies to ensure the log is emitted.src/common/response.ts (4)
16-32: LGTM!Good handling of the edge case where
response.bodyis null, returning a completed capture with zero size immediately.
39-53: LGTM!The
TransformStreamefficiently captures chunks while streaming, with good memory management by clearing the chunks array when the size limit is exceeded.
72-82: Clarify: The timeout only applies before the first chunk is read.The
readTimeoutis designed to handle cases where the response body never starts streaming (e.g., a hung connection). Once reading begins, the timeout will not trigger even if subsequent reads are slow. This is appropriate behavior for streaming responses.
84-93: LGTM!Good use of a new
Responseto proxy the original while allowing body interception. The Bun workaround comment is helpful for understanding the edge case.src/common/consumers.ts (2)
1-5: LGTM!Clean type definition with appropriate optional fields supporting both
undefinedand explicitnullvalues.
17-43: LGTM!The function correctly handles all input variants with proper normalization (trimming, length truncation) and implements a sensible deduplication strategy to reduce payload size on repeated consumer references. The early-return pattern keeps the control flow clear.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/hono/utils.ts (1)
35-42: Consider logging errors for debugging.While the silent catch is intentional for cross-platform compatibility, logging unexpected errors could aid debugging when running outside Cloudflare Workers.
🔎 Suggested improvement with optional logging
export function tryWaitUntil(c: Context, promise: Promise<unknown>) { try { c.executionCtx.waitUntil(promise); } catch (error) { // Execution context is only available in Cloudflare Workers, // but not on other platforms or in unit tests. + // Silently ignore expected errors in non-Cloudflare environments } }Or if you want to aid debugging:
export function tryWaitUntil(c: Context, promise: Promise<unknown>) { try { c.executionCtx.waitUntil(promise); } catch (error) { // Execution context is only available in Cloudflare Workers, // but not on other platforms or in unit tests. + // Uncomment for debugging: console.debug("waitUntil not available:", error); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
LICENSE(1 hunks)eslint.config.js(1 hunks)src/common/bytes.ts(1 hunks)src/common/output.ts(1 hunks)src/hono/utils.ts(1 hunks)tests/hono/app.test.ts(1 hunks)tests/hono/app.ts(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- LICENSE
🚧 Files skipped from review as they are similar to previous changes (3)
- vitest.config.ts
- tsconfig.json
- tests/hono/app.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
tests/hono/app.ts (3)
src/hono/middleware.ts (1)
useApitally(17-96)src/hono/index.ts (2)
useApitally(3-3)setConsumer(4-4)src/hono/utils.ts (1)
setConsumer(24-29)
src/common/output.ts (2)
src/common/consumers.ts (1)
ApitallyConsumer(1-5)src/common/bytes.ts (1)
bytesToBase64(20-27)
src/hono/utils.ts (1)
src/common/consumers.ts (2)
ApitallyConsumer(1-5)consumerFromStringOrObject(17-43)
🔇 Additional comments (13)
src/common/bytes.ts (5)
1-3: LGTM!Standard TextEncoder usage for converting strings to bytes.
5-7: LGTM!Standard TextDecoder usage for converting bytes to strings.
9-18: LGTM!Efficient implementation using
set()for concatenating multiple Uint8Array instances.
20-27: Excellent fix addressing the previous stack overflow concern!The chunked implementation with 4096-byte segments safely handles large byte arrays by avoiding excessive function arguments. The approach is both correct and efficient.
29-36: LGTM!Correct base64 decoding implementation.
src/hono/utils.ts (3)
8-22: LGTM!The endpoint discovery logic correctly filters out middleware and non-standard HTTP methods (ALL, HEAD, OPTIONS).
24-29: LGTM!Correctly stores the consumer in the Hono context with proper falsy value normalization.
31-33: LGTM!Correctly retrieves and normalizes the consumer from the context.
src/common/output.ts (3)
4-27: LGTM!Comprehensive type definition covering all necessary fields for the log payload.
29-38: Excellent fix resolving the previous stack overflow concern!By delegating to the now-chunked
bytesToBase64function, this implementation safely handles large compressed payloads without risking call stack limits.
40-63: LGTM!The implementation correctly handles large payloads by defensively removing bodies when the compressed message exceeds the safe threshold. The toJSON approach provides elegant custom serialization.
eslint.config.js (1)
1-19: LGTM!Standard ESLint flat config setup with reasonable rule customizations for TypeScript. The unused variable patterns appropriately ignore placeholder args and standard error variable names.
tests/hono/app.ts (1)
8-61: LGTM!Well-structured test application covering various scenarios: validation, path/query params, JSON body, error handling, and streaming responses. The comprehensive logging configuration is appropriate for testing the Apitally middleware.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.